Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Improve messages in server:update command #1022

Merged
merged 6 commits into from
Dec 11, 2020
Merged

feat: Improve messages in server:update command #1022

merged 6 commits into from
Dec 11, 2020

Conversation

tolusha
Copy link
Collaborator

@tolusha tolusha commented Dec 7, 2020 •
edited
Loading

Signed-off-by: Anatolii Bazko abazko@redhat.com

What does this PR do?

Improve messages in server:update command

Screenshot/screencast of this PR

Existed Eclipse Che operator: quay.io/eclipse/che-operator:nightly.
New Eclipse Che operator    : quay.io/eclipse/che-operator:nightly.
 ›   Warning: 'chectl' tool is not up to date.
 ›   Update 'chectl' first: 'chectl update next' and then try again.
If you want to continue - press Y:

Existed Eclipse Che operator: quay.io/eclipse/che-operator:nightly.
New Eclipse Che operator    : quay.io/eclipse/che-operator:nightly.
Eclipse Che is already up to date.

Existed Eclipse Che operator: quay.io/eclipse/che-operator:nightly.
New Eclipse Che operator    : docker.io/abazko/operator:local.
 ›   Warning: Eclipse Che operator deployment will be updated with the provided image,
 ›   but other Eclipse Che components will be updated to the nightly version.
 ›   Consider removing '--che-operator-image' to update Eclipse Che operator to the same version.
If you want to continue - press Y:

What issues does this PR fix or reference?

eclipse-che/che#18514

How to test this PR?

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@tolusha tolusha mentioned this pull request Dec 7, 2020
Closed
56 tasks
@tolusha tolusha marked this pull request as ready for review December 8, 2020 15:25
@tolusha
Improve messages in server:update command
b637389 
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Fixes
4c572e4 
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Fixes
7113fd1 
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Fixes
4ec3b94 
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Copy link
Collaborator Author

tolusha commented Dec 10, 2020

/retest

mmorhun
mmorhun approved these changes Dec 11, 2020
View reviewed changes
src/commands/server/update.ts
Comment on lines +144 to +147
cli.warn(`It is not possible to update Eclipse Che to a newer version
using the current '${currentChectlVersion}' version of chectl. Please, update 'chectl'
to a newer version '${latestChectlVersion}' with the command 'chectl update ${chectlChannel}'
and then try again.`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe move into a function with args that returns the message? Then block below will not be duplicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is slightly different.

src/commands/server/update.ts
}

if (!flags.yes && !await cli.confirm('If you want to continue - press Y')) {
this.exit(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be nice to print something into output, like Update cancelled by user

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

src/commands/server/update.ts Outdated
Comment on lines 141 to 142
if (projectName === 'chectl' && latestChectlVersion) {
// suggest update chectl first
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if latestChectlVersion together with suggest update chectl first is very confusing.

Copy link
Collaborator Author

@tolusha tolusha Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it should be !!latestChectlVersion

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we have
ERROR: 141:41 no-extra-boolean-cast redundant double negation in an if statement condition
so return to
if (projectName === 'chectl' && latestChectlVersion) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that we have:

if (latestChectlVersion) {
  // suggest update chectl first
  ...
}

which confuses me a lot

src/commands/server/update.ts Show resolved Hide resolved
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mmorhun, tolusha
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mmorhun mmorhun removed their assignment Dec 11, 2020
@tolusha
Minor improvements
634ed68 
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@openshift-ci-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@tolusha
Fix
ac57fac 
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
@tolusha
Copy link
Collaborator Author

tolusha commented Dec 11, 2020

/retest

@tolusha tolusha merged commit 934da65 into master Dec 11, 2020
@tolusha tolusha deleted the serverupdate branch December 11, 2020 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmorhun mmorhun mmorhun approved these changes

@AndrienkoAleksandr AndrienkoAleksandr Awaiting requested review from AndrienkoAleksandr

@flacatus flacatus Awaiting requested review from flacatus

@l0rd l0rd Awaiting requested review from l0rd

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tolusha @openshift-ci-robot @mmorhun